Refactor Rotated Projection Logic #453
Conversation
This first commit simply tries to lay the foundations for this new class. At the moment, the class does not accomplish anything of substance.
We choose to define this constructor in the RotatedProjWriter.cpp file because we'll be moving the declaration of the `Rotation` struct into the associated header in an upcoming commit
We now track the `Rotation` struct as a member of `RotatedProjWriter`, & explicitly pass it to the functions where it is used.
In more detail: - I deleted the default constructor (to prevent invalid states) - the main constructor now takes `ParameterMap&` as an argument (in preparation for the next commit) - I added a docstring for the class - I adjusted the docstrings of the members (they were unnecessarily verbose for Doxygen)
This takes advantage of the fact that I previously defined the procID variable when we aren't using MPI. It also leverages some the function-like error-checking macros
| // create the filename | ||
| std::string filename = fname_template.format_fname(nfile, "_rot_proj"); | ||
|
|
||
| // it may be a little more explicit to use a switch statement instead of if/elif/else |
There was a problem hiding this comment.
I agree that a switch statement would be clearer if you wanted to change it (but I don't think it's necessary for this PR if you don't feel like it).
| // should we make it an error to specify the: | ||
| // - ddelta_dt parameter when flag_delta != 1 | ||
| // - n_delta parameter when flag_delta != 2 | ||
|
|
There was a problem hiding this comment.
I do think it's a good idea to check that the values of these parameters are valid, at least for the basic things of being positive/nonzero (either in this PR or a future one).
Also, I think it's okay for the n_delta/ddelta_dt parameters to go unused if they're defined with the wrong flag_delta value. But if you're concerned about it being confused, I think printing a warning could also be a good option.
helenarichie
left a comment
helenarichie
left a comment
There was a problem hiding this comment.
I think this looks great and is good to go as-is!
I shared my thoughts on a couple of the questions you left in the comments. If you want to make any of those changes in this PR, go ahead, and I'll review them, but if not, feel free to just resolve my comments.
3 participants
This PR should be reviewed after #433 has been merged.
Overview
This PR essentially makes 3 changes:
RotatedProjWriterclass. This is a callable (e.g. it acts like a function with state) that is tracked withinWriterManager.Rotationstruct is no longer tracked by theGrid3Dclass. NowRotatedProjWritertracks an instance.Rotationinstances are no longer initialized from values in aParametersstruct. Now, values are directly parsed in the constructor ofRotationReview Advice
It may be easier to review this commit-by-commit.
Ouptut_Rotated_Projection_Datainto the callable method of theRotatedProjWriterclass. (This may be a little controversial since I am moving contents between files).